-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sharing saved objects phase 3 #94383
Sharing saved objects phase 3 #94383
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a high-level review of get_references_deep
. I think this will satisfy our needs!
src/core/server/saved_objects/service/lib/get_references_deep.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/get_references_deep.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/get_references_deep.ts
Outdated
Show resolved
Hide resolved
bd111b5
to
2055343
Compare
2055343
to
e85c529
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First real pass through - I haven't tested or reviewed any tests at this point. Nice work so far!
src/core/server/saved_objects/service/lib/update_objects_spaces.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/update_objects_spaces.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/update_objects_spaces.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/update_objects_spaces.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/__mocks__/update_objects_spaces.ts
Outdated
Show resolved
Hide resolved
...k/plugins/security/server/authorization/privileges/feature_privilege_builder/saved_object.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Show resolved
Hide resolved
48121fc
to
e1ec80b
Compare
e1ec80b
to
7f7b6f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only done some high-level smoke testing on this, but wanted to leave you with some comments for discussion before I go on PTO. Looking good so far!
Aside from these comments, I think I've gone as far as I'd like to go in terms of server-side code review before getting the core team's input, specifically on the inclusion of the tags
SOC wrapper. Once we have consensus on that approach, then I'll do a final review with more in-depth testing.
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished reviewing collectMultiNamespaceReferences
. Found nothing problematic - just a few nits and questions 😃
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Show resolved
Hide resolved
7f7b6f7
to
7862c95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review
Overall the implementation looks fine. A few nits and comments.
The only thing we really need to find the best solution for is the current relations/tags dissociation that complexify a lot the implementation and forced to create an additional SO wrapper just to work around the parameter conversion (#94383 (comment))
export interface SavedObjectsCollectMultiNamespaceReferencesOptions | ||
extends SavedObjectsBaseOptions { | ||
/** Whether or not to include tags when collecting references */ | ||
excludeTags?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the discussion started #67380 (comment),
If we were to have an excludeTags
checkbox instead of the current includeTags
in the UI, could we just get rid of this SavedObjectsCollectMultiNamespaceReferencesOptions.excludeTags
property by having the UI just send typesToExclude: ['tag']
when the checkbox is checked?
I know this is poor isolation between the SOM ui and the tagging feature, but if that avoid adding this new SO client wrapper only to toggle an option, it seems like totally worth it.
Also, I agree that now that we no longer have an OSS distribution, we could have the tag
SO type become more 'official', and maybe even put the tag concept/feature into the SOR, which would make the current workaround to just hardcode the typesToExclude
value in the UI even more acceptable for the transition.
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/update_objects_spaces.ts
Outdated
Show resolved
Hide resolved
...k/plugins/saved_objects_tagging/server/saved_objects/tagging_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/spaces/server/routes/api/external/get_shareable_references.ts
Outdated
Show resolved
Hide resolved
@jportner please ping me when you'll want a second pass :) |
7862c95
to
092424e
Compare
@legrego @watson this is ready for another round of reviews (I rebased onto master again and force-pushed -- see the 6 most recent commits for changes since your last reviews). I haven't addressed all of Pierre's feedback yet, but I modified the Secure SOC Wrapper implementation some more and added unit tests to address the deficiencies that I found. Next I'm going to:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on the unit tests! The coverage is great, and they're fairly readable as far as SO tests go.
RE: client-side changes, would it make sense to split the remaining work into a different PR, so that you don't keep this one open for longer than necessary?
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/ensure_authorized.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts
Show resolved
Hide resolved
|
||
externalRouter.post( | ||
{ | ||
path: '/api/spaces/_update_objects_spaces', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure this is on your mental todo list, but for posterity: don't forget the API Docs 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't have any API Docs for the _share_saved_object_add
and _share_saved_object_remove
routes. Since we are moving away from Asciidocs, I plan to only make API Docs for the new _get_shareable_references
and _update_objects_spaces
routes using the new docs system. I'll add that in a follow-on PR along with other sharing-saved-objects-related guidance 👍
...ins/spaces/public/share_saved_objects_to_space/components/share_to_space_flyout_internal.tsx
Show resolved
Hide resolved
Thanks!
I like that idea. The currently included client-side work should be enough to rip out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addition to SavedObjectsCreatePointInTimeFinderOptions LGTM
ML changes tested. LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looked at core's changes. Implementation did not change much.
LGTM, just a few remarks and questions
hidden: true, | ||
hidden: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we now need this type to be visible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we call collectMultiNamespaceReferences
, for every object that we collect, we also check to see if it has any legacy URL aliases in any other spaces, and return that to the consumer via the spacesWithMatchingAliases
field.
This search relies on the SOR's find
method, so we can't do it if the object type is hidden, unless we want to add some sort of workaround. I didn't want to add anything else to the growing list of find
options, so this seemed like the best approach.
* Note: if options.purpose is 'updateObjectsSpaces', it must be a shareable type (in other words, the object type must be registered with | ||
* the `namespaceType: 'multi'`). | ||
*/ | ||
export interface SavedObjectsCollectMultiNamespaceReferencesObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: add @public
to all public types/interfaces of this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think I had asked him to remove @public
🙈. I was under the impression that it wasn't required -- anything that's eventually exported out of src/core/server
should automatically be treated as "public", right? Is there another reason for including this tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@legrego if we don't include this tag, the top-level docs (src/core/public/public.api.md
and src/core/server/server.api.md
) include a warning before the interface, example:
// Warning: (ae-missing-release-tag) "SavedObjectsCollectMultiNamespaceReferencesObject" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public
export interface SavedObjectsCollectMultiNamespaceReferencesObject {
...
Sorry I missed this, @pgayvallet! I'll add the @public
tags back in 👍
async collectMultiNamespaceReferences( | ||
objects: SavedObjectsCollectMultiNamespaceReferencesObject[], | ||
options?: SavedObjectsCollectMultiNamespaceReferencesOptions | ||
): Promise<SavedObjectsCollectMultiNamespaceReferencesResponse> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is not really introducing it, but we have an inconsistency on the space
vs namespace
naming. eg collectMultiNamespaceReferences
versus updateObjectsSpaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was my understanding that we want to eventually get rid of namespace
terminology in favor of space
now that we aren't shipping an OSS distribution. I agree this is confusing, especially because this is the first mention of "Spaces" in the Core codebase. I'm happy to change this to updateObjectsNamespaces
if you think that would be better for now.
x-pack/plugins/spaces/server/routes/api/external/update_objects_spaces.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, mostly nits and a couple of small items I think we should tweak as we prepare for The Merge™️
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/collect_multi_namespace_references.test.ts
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Show resolved
Hide resolved
x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/spaces/server/routes/api/external/get_shareable_references.ts
Show resolved
Hide resolved
x-pack/plugins/spaces/server/routes/api/external/update_objects_spaces.ts
Show resolved
Hide resolved
Keeping the new unit tests, though. Modified them accordingly.
This was an oversight that I just noticed while updating TSdocs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest changes to SO code LGTM! 🚀
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great work as usual @jportner! 🎉
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Joe Portner <[email protected]>
Resolves #67380
Changes
This PR primarily makes changes to the server-side SavedObjectsRepository (SOR) and SavedObjectsClient (SOC) methods.
1. Added new SOR/SOC methods:
collectMultiNamespaceReferences
-- Given an list of object types/IDs, this will collect all outbound multi-namespace references (including transitive references) and return them. It also checks for legacy URL aliases in other spaces and includes that information in the result./api/spaces/_get_shareable_references
updateObjectsSpaces
-- Given a list of object types/IDs, spaces to add, and spaces to remove, this will update the spaces for each object./api/spaces/_update_objects_spaces
I implemented a new approach for saved objects authorization in these methods. First we fetch the object(s) in question, then we check privileges for all spaces (including any spaces those objects currently exist in). This has the benefit of reducing the amount of privilege checks we need to make (we don't need an additional privilege check to redact object namespaces). This is an implementation detail that is not visible to consumers.
For
collectMultiNamespaceReferences
, if the user is not authorized to access a requested object, they will encounter a 403 error. If they are not authorized to access a referenced object, that will be silently filtered out of the results. This is consistent with our existing authorization checks for exporting saved objects with references. See unit tests and API integration tests for specific examples.2. Removed outdated SOR/SOC methods:
I removed the
addToNamespaces
anddeleteFromNamespaces
methods, these are superseded by the newupdateObjectsSpaces
method. I updated existing client code in the "spaces" and "ml" plugins to use the new method.3. Updated Copy-to-Space behavior
The
copySavedObjectsToSpaces
API has been improved for usage with multi-namespace types. Previously, if you attempted to copy an object to another space where it already exists, it would attempt to do so and return a conflict error. Then you would be able to resolve the conflict error by overwriting the same object with itself. While this technically worked, it was a confusing and poor user experience. Now if you attempt to copy object(s) to space(s) where they already exist, those will be skipped.Note: we made a conscious decision not to do the same for
resolveCopySavedObjectsToSpacesConflicts
, as it's a lot of extra testing surface for less benefit. Technically one user could attempt a copy, then another user could share one of the affected objects to a destination space before the first user gets a chance to resolve the conflicts -- in this case, if the user had chosen to overwrite the destination object, it would be harmless and nothing would actually change.What is not included
These will be addressed in a follow-on PR:
resolve
to result in an alias conflict scenario. Instead, at share time, the user must decide to skip the changes, or to disable the legacy URL aliases in the destination space(s).force
optionresolve
method -- this should have been done in Phase 2.5 but it was missed because it does not rely on the same type definitions as the server-side SOC/api/spaces/_get_shareable_references
and/api/spaces/_update_objects_spaces
in the new docs systemresolve
method (to track how many times legacy URL aliases are used in practice, and to track how many legacy URL aliases are disabled)Testing
To manually test
collectMultiNamespaceReferences
functionality:It registers the
toy
saved object type (which is shareable), and allows you to easily generate new objects for testing._get_shareable_references
API returns a list of spaces where this object has conflicting aliases.To manually test
updateObjectsSpaces
functionality: